-
Notifications
You must be signed in to change notification settings - Fork 40
Customer account rc doc - support doc generate #2768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Customer account rc doc - support doc generate #2768
Conversation
79048ab
to
1b2c949
Compare
@@ -0,0 +1,244 @@ | |||
/* eslint-disable no-undef, no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the script re-usable and pass in the right paths instead of duplicating? It would be better for maintainance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't copy paste this script. Bring it up layers and make it reusable for everyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted all the shared functions, and let each service construct their own build steps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great progress but I think there is more to do. The directory customer-account
could be an argument passed into the CLI that would remove a lot of duplicate code. I think there is a lot of duplication that still should be consolidated. extensions api version should be shared...
I think we should really aim at making one script with arguments that works for all surfaces instead of splitting it into reusable functions. I understand the time pressure folks are under so I am willing to be flexible but I think this still needs a second pass.
"Provides information about the company of the authenticated business customer. The value is `undefined` if a business customer isn't authenticated.", | ||
type: 'UseAuthenticatedAccountPurchasingCompanyGeneratedType', | ||
}, | ||
// { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removing all the hooks for now, and will re-add them later once we have the hooks
@@ -5,7 +5,7 @@ | |||
"docs:admin": "node ./docs/surfaces/admin/build-docs.mjs", | |||
"docs:checkout": "bash ./docs/surfaces/checkout/build-docs.sh", | |||
"docs:point-of-sale": "bash ./docs/surfaces/point-of-sale/build-docs.sh", | |||
"docs:customer-account": "bash ./docs/surfaces/customer-account/build-docs.sh" | |||
"docs:customer-account": "node ./docs/surfaces/customer-account/build-docs.mjs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
follow line 5 admin's behvaiour
@@ -0,0 +1,128 @@ | |||
export interface CustomerAccountActionProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copy and combined from all the components' d.ts file for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no avatar for the 1st rc version
@@ -35,11 +35,6 @@ const data: ReferenceEntityTemplateSchema = { | |||
codeblock: { | |||
title: 'Basic CustomerAccountAction', | |||
tabs: [ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all react examples for components as there is no ui-extensions-react
@@ -106,10 +106,6 @@ export interface CustomerAccountExtensionTargets { | |||
FullPageApi, | |||
StandardComponents | |||
>; | |||
'CustomerAccount::KitchenSink': RenderExtension< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the test KitchenSink
target
aa5819d
to
883ed8d
Compare
@@ -0,0 +1,244 @@ | |||
/* eslint-disable no-undef, no-console */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make the script re-usable and pass in the right paths instead of duplicating? It would be better for maintainance.
.replace(/'/g, "'"); | ||
}; | ||
|
||
const htmlWrapper = (htmlString, layout) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this won't work for customer accounts. Maybe we should have a way to exclude previews. Thought @Fionoble @alex-page ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be renamed to adminHTMLWrapper
and the templates below could be updated to include admin
as well and use the adminHTMLWrapper
. It's quite a simple adjustment but will ensure they can add their own templates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted some shared functions and make transformJson customizable 0459c0c
await fs.cp(generatedDocsPath, shopifyDevDBPath, {recursive: true}); | ||
}; | ||
|
||
const generateExtensionsDocs = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should be brought into the admin docs generator and be named something with customer accounts name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extracted some shared functions and make transformJson customizable 0459c0c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great prototype but we should consolidate here instead of duplicate 90% of the work. Happy to support the consolidation but it's likely just adding prefixes to ensure docs do not overlap and have clear functions for each area.
rootPath, | ||
generatedDocsDataFile, | ||
generatedStaticPagesFile, | ||
transformJson: (filePath) => transformJson(filePath, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now this is an optional function, admin will pass it
outputDir, | ||
rootPath, | ||
generatedDocsDataFile, | ||
transformJson: (filePath) => transformJson(filePath, false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not pass generatedStaticPagesFile
here so that generateFiles
will know do not replace non-existing static json file
@@ -0,0 +1,72 @@ | |||
/* eslint-disable no-undef, no-console */ | |||
import childProcess from 'child_process'; | |||
import fs from 'fs/promises'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all extracted from admin's buold-docs scripts. All these functions will only use parameters, no global variables any more
replaceValue: '', | ||
}); | ||
|
||
if (transformJson) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
transformJson
become optional
Hey @alex-page and @vividviolet , I
can you help to give another review? thanks example test video here https://screenshot.click/15-56-qwuux-c1885.mp4 |
1fc0607
to
a61ae38
Compare
Good progress. I think more should be done but folks are racing for deadlines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as it looks good on my end. I guess there's still some concerns about sharing as much logic as possible. We can look to improve the script even more, although not sure how deep we want to go into that for this PR
69c63db
into
customer-account-rc-doc---remove-react-examples
Background
Customer account rc version now supports doc generate.
Solution
Follow admin's behvaiour
🎩
Demo video: https://screenshot.click/15-01-yjwfa-6clit.mp4
Sorry Spin is not working anymore...
Checklist